-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: bump golangci-lint version in build image #3195
Conversation
Old versio of golangci-lint has a memory leak causing it to crash with oom in recent go versions. Updated golangci-lint version. Fixed some lint errors.
@@ -55,22 +55,22 @@ func (*deleteRDSSnapshotFunc) Name() string { | |||
return DeleteRDSSnapshotFuncName | |||
} | |||
|
|||
func deleteRDSSnapshot(ctx context.Context, snapshotID string, profile *param.Profile, dbEngine RDSDBEngine) (map[string]interface{}, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes an unparam
linter warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KastenMike I am assuming this was flagged by a linter as part of the upgrade. @hairyhum can confirm.
If that's the case, there are 2 options:
- Put all the changes in a single PR, like this one; or
- 2 PRs in this order: the first one addressing the linter warnings, and the second one upgrading the linter version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upgrading linter version is a github action, which I've already run, so it's just a matter of getting it to the master
codebase
@@ -134,7 +134,7 @@ func createVolumeSnapshot(ctx context.Context, tp param.TemplateParams, cli kube | |||
} | |||
wg.Wait() | |||
|
|||
err := fmt.Errorf(strings.Join(errstrings, "\n")) | |||
err := fmt.Errorf("%s", strings.Join(errstrings, "\n")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this should use errors.New
err := fmt.Errorf("%s", strings.Join(errstrings, "\n")) | |
err := errors.New(strings.Join(errstrings, "\n")) |
Question unrelated to this PR why are newlines (\n
) being used inside the error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe errkit, but I was trying to keep the changes minimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to errkit because we already have it in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG. See minor comment
Change Overview
Old version of golangci-lint has a memory leak causing it to crash with oom in recent go versions.
Updated golangci-lint version in build image.
Fixed lint errors.
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan